Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make websocket server to send ping frame to client #459

Merged
merged 3 commits into from
Jan 9, 2021
Merged

make websocket server to send ping frame to client #459

merged 3 commits into from
Jan 9, 2021

Conversation

franckchen
Copy link

@franckchen franckchen commented Jan 7, 2021

References

try to fix #458

Code changes

User-facing changes

Backwards-incompatible changes

Chores

  • linted
  • tested
  • documented
  • changelog entry

@franckchen franckchen changed the title make websocket server to send ping frame to client(#458) make websocket server to send ping frame to client Jan 7, 2021
@krassowski
Copy link
Member

Thank you for the PR! Our test mocks were not prepared to handle more advanced features of the websocket mixin, but I just improved them and added a test case to make sure it works. I will merge if the CI tests pass.

@krassowski
Copy link
Member

I wonder if we also need pong from the other side.

@krassowski krassowski requested a review from bollwyvl January 7, 2021 21:31
@krassowski
Copy link
Member

There is an interesting possibly pyls-related failure on Python 3.6 on Ubuntu, but not on Mac nor Win:

Element 'css:div.lsp-statusbar-item' did not get text 'Fully initialized' in 1 minute.

Also teardown failed:
Phrases found in /home/runner/work/jupyterlab-lsp/jupyterlab-lsp/atest/output/linux_36_1/lab.log: {'pyls_jsonrpc.endpoint - Failed to handle request': True}

@krassowski krassowski closed this Jan 8, 2021
@krassowski krassowski reopened this Jan 8, 2021
@krassowski
Copy link
Member

(The close/reopen is to restart the CI)

@franckchen
Copy link
Author

from the other side

No. I think we dont need care about pong. Web browser will automatically reply pong message, dont require developer to code about it. Browser even dont provide api for developer to send pong to server.I dont know if this is what you are worried about.

@pytest.mark.asyncio
async def test_ping(handlers):
"""see https://github.com/krassowski/jupyterlab-lsp/issues/458"""
a_server = "pyls"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just do not think it is needed to run this check for all language servers (just wanted to have something to pass to ws_handler.open()) but did not want to create a mock either.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, yeah, maybe we need a real pygls mock server at some point :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am fine with bringing pygls as a mock dependency in the future.

@@ -26,6 +26,7 @@ def open(self, language_server):
self.language_server = language_server
self.manager.subscribe(self)
self.log.debug("[{}] Opened a handler".format(self.language_server))
super().open()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very good catch. might we even want to super.open first, before any overloaded behavior?


manager.initialize()

assert ws_handler.ping_interval > 0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are looking good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants